-
Notifications
You must be signed in to change notification settings - Fork 27
deploy command support for drafts #679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
| ["voila", get_dir(join("pip1", "dummy.ipynb"))], | ||
| ] | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing, but in practice it produces product of args * flag thus testing every deploy command with both flag=True and flag=False
| return ( | ||
| response.json_data | ||
| if response.status and response.status == 200 and response.json_data is not None | ||
| if response.status and response.status >= 200 and response.status <= 299 and response.json_data is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/content/X/deploy responds with a 201 and a JSON body
| for key, value in headers.items(): | ||
| logger.debug("--> %s: %s" % (key, value)) | ||
| logger.debug("Body:") | ||
| logger.debug("--> %s" % (body if body is not None else "<no body>")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this extra logging of the Request body when in debug (-vvv) for convenient to see what the client exchanged with the server.
| args.extend(["--cacert", cacert]) | ||
| if insecure: | ||
| args.extend(["--insecure"]) | ||
| return args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convenience so that I can do
args = apply_common_args(["deploy", command, target], server="http://fake_server", key="FAKE_API_KEY")
instead of
args = ["deploy", command, target]
apply_common_args(args, server="http://fake_server", key="FAKE_API_KEY")
semantically is not the best that it both modifies in place and also returns the value as it might be confusing that the two are the same, but given that it's just a test helper it shouldn't be a big deal.
| app_bundle = self.app_upload(app_id, tarball) | ||
|
|
||
| task = self.app_deploy(app_id, app_bundle["id"]) | ||
| task = self.content_deploy(app_guid, app_bundle["id"], activate=activate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only use of app_deploy, it might be worth clearing it up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app_deploy is in api, there were other APIs that were not used. I think it makes sense to cover all apis in the API module, independently from the ones we use. But if we are strong about removing the ones we don't use I'm ok with removing them.
marcosnav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation wise, looks good to me, a couple non blocker items:
- It'd be nice to clean up the
app_deploy - I'm starting to question the use of the term "draft"
|
I thought we were calling this |
The name in Connect is during design there were a few discussions about glossary and one of the terms for the idea of deploying a new application with 0 active bundles was to deploy a draft. As a |
|
I think "preview deployment" is well-enough established as a noun in similar contexts (e.g. netlify, cloudflare, vercel, etc) that |
Agreed that is common enough, but "preview" was moved to "activate" as a term in the API. Also vercel itself uses both terms to more or less convey a similar concept 😅 Generally speaking, the term "draft" seems to be attached to a persistent state that is not public (a version of a content that I never published but will sit there forever) |
|
Let's go with what's here in the code now (which IIRC, is |
Intent
Allows to deploy content without activating it.
Type of Change
Approach
New command line option that injects the parameter only when necessary, this way it will be backward compatible with previous API when the parameter is not provided.
Automated Tests
test_deploy_draftruns the command with all possible permutations of content and activate=True/FalseDirections for Reviewers
Currently the deploy doesn't output the url where the draft can be seen, that's planned for a follow-up PR as the two can be done in isolation.
Checklist